Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warnings for web3.sha3, middleware_stack, and python 3.5 #1020

Merged
merged 6 commits into from
Sep 19, 2018

Conversation

dylanjw
Copy link
Contributor

@dylanjw dylanjw commented Aug 27, 2018

What was wrong?

Related to Issue #722

How was it fixed?

I lumped together a few of the straight forward deprecation warnings:

Cute Animal Picture

image

web3/__init__.py Outdated
"Support for Python 3.5 will be removed in web3.py v5",
category=DeprecationWarning,
stacklevel=2)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to have both warnings print in the case that the python version is less than 3.5, so that the user doesnt have to update to 3.5 to learn that support for 3.5 will be dropped soon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first one is an exception and will exit the python process before hitting this new warning.... so I don't think this behaves as you expect.

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty straight forward. Pretty quick scan.

web3/__init__.py Outdated
"Support for Python 3.5 will be removed in web3.py v5",
category=DeprecationWarning,
stacklevel=2)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first one is an exception and will exit the python process before hitting this new warning.... so I don't think this behaves as you expect.

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is to be merged in before v4 branches off...

We should leave at least a few smoke tests that the old deprecated names still work.

web3/main.py Outdated
@@ -132,11 +138,12 @@ def providers(self, providers):
self.manager.providers = providers

@staticmethod
@deprecated_for("This method has been renamed to web3.keccak")
@apply_to_return_value(HexBytes)
def sha3(primitive=None, text=None, hexstr=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that in the future we're going to change something in one implementation or the other, instead of both. Can we just make the deprecated version an alias to the new one, like:

def sha3(...):
   return Web3.keccak(...)

I think it's okay to have the error message reference keccak instead of sha3 during deprecation time.

@dylanjw
Copy link
Contributor Author

dylanjw commented Sep 5, 2018

Referencing the conversation at the end of #1039. The middleware_stack deprecation warning may need to change or be broken out of this PR.

@dylanjw
Copy link
Contributor Author

dylanjw commented Sep 18, 2018

I remove the commit renaming middleware_stack to middleware_onion, and added a few tests to ensure the deprecated method, Web3.sha3 works as expected.

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG when CI finishes green 👍

@dylanjw dylanjw merged commit a8a5032 into ethereum:master Sep 19, 2018
johnsBeharry added a commit to peakshift/web3.py that referenced this pull request Oct 21, 2018
The cryptographic hashing method sha3 was renamed to keccak in PR ethereum#1020 and thus deprecated but the docs were not updated to reflect this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename sha3 to keccak Rename middleware_stack to middleware_onion
3 participants